feat(platform-wallet): persistor backports — retry-safe flush, prepare_cached writers, functional load()#3643
Conversation
Intermediate backport step toward the SQLite persister load() path. Non-breaking on this branch: the rs-platform-wallet public surface is unchanged versus the base branch — later commits keep ClientStartState at its base two-slot shape (platform_addresses / wallets). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reworks `flush_inner` so transient SQLite failures (BUSY/LOCKED) restore
the buffered changeset for the caller to retry, while fatal failures
drop the buffer and surface verbatim. Closes the data-loss window where
a failed flush would silently discard staged state.
Changes:
- `Buffer::take_for_flush` + `restore` (additive); `drain` is a
one-cycle deprecated alias.
- `WalletStorageError::FlushRetryable { wallet_id, source }` variant
with hex-rendered display + rusqlite source for the retry path.
- `WalletStorageError::is_transient()` + `error_kind_str()` — both
wildcard-free so adding a future variant is a hard compile error.
Enum intentionally NOT marked `#[non_exhaustive]`.
- `flush_inner` splits into `write_changeset_in_one_tx` (clean SQL
path, returns `WalletStorageError`) + `handle_flush_error`
(classification, restore, structured tracing).
- Test affordance: `force_next_flush_to_fail(WalletStorageError)` on
the persister, gated `#[cfg(any(test, feature = "test-helpers"))]`.
- `PlatformWalletChangeSet::populated_field_count` for tracing fields.
Tests (TDD; all green):
- TC-P2-001 happy-path one-tx + second flush is no-op
- TC-P2-002 transient → FlushRetryable + retry succeeds
- TC-P2-003 store-during-failed-flush LWW merge
- TC-P2-004 fatal failure wipes the buffer
- TC-P2-005 is_transient table (every variant; wildcard-free)
- TC-P2-006 Immediate mode surfaces FlushRetryable
- TC-P2-007 structured `tracing::warn!` on restore
- TC-P2-010 boundary mapping FlushRetryable → PersistenceError::Backend
- Two new buffer.rs unit tests for take/restore LWW + empty-slot insert
Dev-deps: `tracing-test = "0.2"` (no-env-filter), `serial_test = "3"`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
<sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Lifts every in-loop `tx.execute(SQL, ...)` in the schema modules into a `tx.prepare_cached(SQL)` outside the loop, then `stmt.execute(...)` inside. SQLite parses + plans each statement once per `Connection` lifetime; subsequent flushes hit the cache. No public-API change, no schema migration. Touched: identities, contacts, asset_locks, accounts, dashpay, identity_keys, token_balances, platform_addrs, core_state, wallet_meta. Read-only single-shot SELECTs in load/list paths stay on `prepare` (per FR-P1-1) — `tests/sqlite_compile_time.rs::tc_p1_003` enforces the boundary via an explicit allow-list. Tests added: - TC-P1-004 (`tc_p1_004_cache_scope_under_heavy_reuse`): 60 store+flush cycles past SQLite's default 16-statement cache to exercise LRU. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…acts, asset_locks Rewrites `SqlitePersister::load()` to populate every wired-up slot on `ClientStartState`: `platform_addresses` (already covered), plus the new `identities`, `contacts`, `asset_locks`. `wallets` stays empty pending upstream `Wallet::from_persisted` — surfaced via the structured `wallets_pending_rehydration` field on the load summary log. Per-area readers added: - `schema::identities::load_state` — decode `entry_blob` rows into `IdentityManagerStartState`; route by `identity_index = Some(_)` (wallet bucket) vs `None` (out-of-wallet bucket); reconstruct `ManagedIdentity` from `IdentityEntry` via a fresh `IdentityV0`. - `schema::contacts::load_state` — three SELECTs over `contacts_sent`/`recv`/`established`; rebuild typed keys (`SentContactRequestKey`, `ReceivedContactRequestKey`, `EstablishedContact`). - `schema::asset_locks::load_state` — wraps `list_active` with per-row corruption tolerance; returns the `AssetLocksByAccount` shape directly compatible with `ClientStartState::asset_locks`. Each reader returns `(state, skipped_rows)` and emits a structured `tracing::warn!` with `wallet_id` + `table` + `error` for every row that fails to decode. The skipped count is folded into the load summary; load itself returns `Ok(state)` with the partial result (per OQ-P4-3 — soft-signal, callers don't need to swallow an error to recover state). `load()` summary log carries six counters per FR-P4-4: `wallets_seen`, `addresses_loaded`, `identities_loaded`, `contacts_loaded`, `asset_locks_loaded`, `wallets_rehydrated` (= 0 today). Empty fields appear as integer `0` so log parsers can rely on the schema. Tests (TDD; all 9 in sqlite_load_reconstruction green): - TC-P4-003 identities round-trip per wallet - TC-P4-004 contacts round-trip per wallet - TC-P4-005 asset_locks bucketed (wallet, account, outpoint) - TC-P4-006 wallets_pending_rehydration log fires with count - TC-P4-007 summary log carries every counter (including zeros) - TC-P4-008 corruption: truncated bincode → partial state + WARN - TC-P4-010 empty database → defaults, zero corruption warnings Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
… load() Adds two README sections covering the new behavior: - "Flush semantics": explains the succeed-or-restore contract on transient SQLite failures, the FlushRetryable marker callers should grep for, and the ban on `#[non_exhaustive]` for is_transient gating. - "load() reconstruction": tabulates which `ClientStartState` slots load now populates, calls out `#[non_exhaustive]` on the struct, and documents the corruption-tolerance contract + structured load-summary log fields. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Rolls up the error/classification fixes from the QA fix wave: - `WalletStorageError::FlushRetryable` Display now embeds the variant name `FlushRetryable` so operators grepping production logs can match on the variant directly (TC-P2-010 restored to assert the variant marker substring). Adds a "use exponential backoff; do NOT tight-loop" warning to the rustdoc. - `WalletStorageError::LoadIncomplete` is `#[deprecated]` with a pointer to the future `try_load()` API. The variant stays in the enum so the wildcard-free match in `is_transient` / `error_kind_str` keeps its compile-time exhaustiveness gate; both callers carry `#[allow(deprecated)]` to acknowledge that. - `tc_p2_005_is_transient_table` is rewritten as a wildcard-free `match` over `&WalletStorageError` (was a runtime `Vec`); a future variant landing on the enum now refuses to compile in BOTH the impl AND the test, matching the spec's two-file gate. - `LockPoisoned` arm in `is_transient` carries a TODO(qa) pointing at the deferred TC-P2-008 mutex-poison test (race-prone per spec; only manual / table-driven coverage today). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…dup + chore cleanups Rolls up the remaining QA fix-wave items beyond error classification: - **load() is constant-query w.r.t. wallet count** (FR-P4-6). Adds `load_all(&Connection) -> BTreeMap<WalletId, _>` to each per-area reader (`platform_addrs`, `identities`, `contacts`, `asset_locks`), each running ONE bulk SELECT (or three for the contacts subtables) ordered by `wallet_id` and grouping in memory. `SqlitePersister::load()` now calls `load_all` once per area and merges; the per-wallet loop is gone. New TC-P4-012 (`tc_p4_012_load_query_count_constant`) verifies via `sqlite3_trace_v2` that the query count for N=1 wallets equals the count for N=10 wallets. - **asset_locks decode helper.** `decode_row(...) -> (account_index, OutPoint, TrackedAssetLock)` is now shared between `list_active`, `load_state`, and `load_all`. The duplicate body Marvin flagged (DEDUP-001) is gone; `load_state` adds the corruption-tolerance loop wrapper around `decode_row` returns. Docstrings updated. - **load() doctest** is now a runnable `# fn main()` example exercising open → empty load → assert per-slot defaults; runs under `cargo test --doc -p platform-wallet-storage`. Replaces the `text` block that previously referenced a non-existent `WalletPersister`. - **`__test-helpers` rename.** The crate-private test feature is renamed from `test-helpers` to `__test-helpers` per Cargo's double-underscore convention for "MUST NOT enable from downstream" features. README + dev-deps + every `cfg` updated. - **README** drops the deprecated `SqlitePersisterError` alias in favor of canonical `WalletStorageError`. - **Buffer::drain `since`** matches the workspace `3.1.0-dev.1` and notes the planned 3.2.0 removal target. - **LOAD_UNIMPLEMENTED rustdoc** now describes the actual tracing-only soft-signal behavior (was claiming the variant is returned). - **M7 TODO** at `handle_flush_error`'s fatal branch documents the deferred TC-P2-008 mutex-poison test (race-prone per spec). - **rusqlite `trace` feature** enabled to support the query-count test's `Connection::trace_v2` callback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| /// `contacts_*` tables. Rows that fail to decode are skipped — the | ||
| /// skip count is returned so callers can surface it via the `load()` | ||
| /// summary log. | ||
| pub fn load_state( |
There was a problem hiding this comment.
In all load functions, we should fail on error, not continue.
There was a problem hiding this comment.
Done in 3aaf260d43. schema/contacts.rs:128-192 (load_state): every row decode propagates via ?; decode_pair_key (:216-222) returns typed WalletStorageError::BlobDecode for non-32-byte ids; blob::decode errors propagate. No warn-skip, no continue, no .ok(). Verified fail-hard in 4fb263b2 (test tc_p4_008b asserts Err(BincodeDecode) / Err(BlobDecode)).
🤖 Co-authored by Claudius the Magnificent AI Agent
| /// subtable), grouped in memory by wallet id. Returns per-wallet | ||
| /// `(ContactsStartState, skipped_rows)` so the persister `load()` path | ||
| /// is constant-query w.r.t. wallet count (FR-P4-6 — no N+1). | ||
| pub fn load_all( |
There was a problem hiding this comment.
Let's just use load_state() in a loop here. No need to repeat that logic.
There was a problem hiding this comment.
Done in 3aaf260d43, finalized in 4fb263b2. The duplicated bulk-decode body is gone — load_state is the single source of truth. contacts::load_all was deleted entirely in 4fb263b2 (genuinely unused once load() returns the base ClientStartState); only platform_addrs::load_all remains as the wallet_meta::list_ids → load_state loop driving the live load() path. No repeated logic anywhere.
🤖 Co-authored by Claudius the Magnificent AI Agent
| /// memory by wallet id. Returns per-wallet | ||
| /// `(AssetLocksByAccount, skipped_rows)` so the persister `load()` | ||
| /// path is constant-query w.r.t. wallet count (FR-P4-6 — no N+1). | ||
| pub fn load_all( |
There was a problem hiding this comment.
Let's just use load_state() in a loop here. No need to repeat that logic.
There was a problem hiding this comment.
Done in 3aaf260d43. schema/asset_locks.rs:85-109 decode_row is the single shared decoder for list_active and load_state — fail-hard on malformed outpoint/blob/out-of-range account index. asset_locks::load_all deleted in 4fb263b2 (dead once load() returned base state). No repeated logic.
🤖 Co-authored by Claudius the Magnificent AI Agent
| /// `(IdentityManagerStartState, skipped_rows)` so the persister | ||
| /// `load()` path is constant-query w.r.t. wallet count (FR-P4-6 — no | ||
| /// N+1). | ||
| pub fn load_all( |
There was a problem hiding this comment.
Let's just use load_state() in a loop here. No need to repeat that logic.
There was a problem hiding this comment.
Done in 3aaf260d43. schema/identities.rs:79-113 load_state fails hard on decode error; tombstoned rows continue (:94-96) — a logical delete, not corruption tolerance, documented :71-73. identities::load_all deleted in 4fb263b2. No duplicated bulk logic.
🤖 Co-authored by Claudius the Magnificent AI Agent
| /// future `try_load() -> Result<(ClientStartState, | ||
| /// Vec<LoadWarning>), _>` API that will hand callers a typed value | ||
| /// instead of forcing them to consume `tracing` events. | ||
| #[deprecated( |
There was a problem hiding this comment.
Done in 3aaf260d43. WalletStorageError::LoadIncomplete and every reference deleted (workspace grep = 0). error.rs: is_transient (:268-311) and error_kind_str (:316-350) remain exhaustive and wildcard-free; #[non_exhaustive] deliberately NOT added (rationale :264-267). tc_p4_011 + sqlite_error_classification.rs guard this at compile time.
🤖 Co-authored by Claudius the Magnificent AI Agent
| /// the structured field `wallets_pending_rehydration` on the | ||
| /// `tracing::info!` summary. | ||
| /// | ||
| /// Corruption tolerance: rows that fail to decode are skipped |
There was a problem hiding this comment.
We don't want to have corruption tolerance. Refactor whole crate to fail on corruption.
There was a problem hiding this comment.
Crate refactored to fail-hard in 3aaf260d43. persister.rs:659-683 (load()): every reader call is ? + map_err(PersistenceError::from) — no .ok(), no partial Ok, no skipped_rows. Stale README documenting the old tolerant design was rewritten in 4fb263b2 (README.md:43-67 now states fail-hard reality). 227 tests pass.
🤖 Co-authored by Claudius the Magnificent AI Agent
| /// | ||
| /// Persister-side reader: | ||
| /// `platform_wallet_storage::sqlite::schema::identities::load_state`. | ||
| pub identities: BTreeMap<WalletId, IdentityManagerStartState>, |
There was a problem hiding this comment.
Let's remove newly added identities, contacts and asset_locks; adding new fields to the rs-platform-wallet is out of scope of this PR.
There was a problem hiding this comment.
Reverted in 64bb12d88, fully closed in 4fb263b2. git diff <base>..HEAD -- packages/rs-platform-wallet is entirely empty — the whole crate is byte-identical to base, not just client_start_state.rs. The earlier additive helper populated_field_count was removed from rs-platform-wallet and reimplemented as a private free fn in storage (persister.rs:706) using only the existing public PlatformWalletChangeSet fields — zero net-new public rs-platform-wallet symbol. tc_p4_011 compile-time-asserts the base shape. Thread #7 is fully satisfied.
🤖 Co-authored by Claudius the Magnificent AI Agent
Drop the non-base ClientStartState extensions (PR #3643 thread #7): restore the plain (no #[non_exhaustive]) two-slot struct carrying only platform_addresses + wallets, delete the ContactsStartState type and its module, and tidy the now-redundant `..` rest patterns at the three production destructure sites. Post-revert the rs-platform-wallet public API equals the base branch, so this is non-breaking. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address PR #3643 reviewer threads #1–#6 and the four architecture adjustments: - Delete the unused LoadIncomplete error variant and every reference, keeping all error matches wildcard-free (threads #5). - Make every per-row decode failure and non-32-byte wallet_id blob a typed Err (BlobDecode / InvalidWalletIdLength) instead of a warn-skip or [0u8;32] zero-bucket — corruption is never silently dropped (threads #1, #6). list_ids now fails hard on bad ids. - Collapse contacts/identities/asset_locks/platform_addrs load_all into thin loops over wallet_meta::list_ids -> load_state, removing the duplicated bulk-decode bodies (threads #2, #3, #4). Each collapsed load_all documents that FK triggers prevent orphans so the id-union is intentionally not restored (adjustment #3). - Add a crate-internal ContactsRecords struct for contacts readers so the deleted ContactsStartState type is not re-exported (adjustment #2); expose a __test-helpers wrapper for coverage. - Rewrite persister load() to return the base ClientStartState (platform_addresses only, wallets empty, wallets_pending_rehydration info kept); drop skipped_rows accounting and the partial-state warn. - Propagate the buffer-restore Result in handle_flush_error instead of swallowing it (thread-driven). - Update the load() doctest and tests: identities/contacts/asset_locks readers covered via direct load_state calls, corrupt-row inputs now assert Err, ClientStartState compile-time test asserts the base shape (adjustment #1). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… fix stale docs QA-002: rewrite the asset_locks `decode_row` and `list_active` doc comments — they still claimed corruption-tolerant per-row skip behaviour, contradicting the hard-fail code. They now state the typed-error fail-hard contract, consistent with `load_state`. QA-003: add direct corrupt-row tests for `contacts::load_state` and `asset_locks::load_state` (TC-P4-008b/008c) — a garbage blob asserts `WalletStorageError::BincodeDecode`, a non-32-byte id / non-36-byte outpoint column asserts `WalletStorageError::BlobDecode`. Add TC-P4-008d covering `wallet_meta::list_ids` rejecting a non-32-byte stored wallet_id with `WalletStorageError::InvalidWalletIdLength` (the path where a malformed id actually surfaces, since per-area readers take a typed &WalletId). QA-004: tighten the existing identities corruption assertion to the specific `BincodeDecode` variant instead of a bare `.is_err()`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_all, stale README PROJ-005: PlatformWalletChangeSet::populated_field_count() did not exist in the base branch — it was net-new public rs-platform-wallet API (violates thread #7). Only the storage persister consumed it. Removed it from rs-platform-wallet and reimplemented the same count as a private free fn in storage's persister.rs using the existing public PlatformWalletChangeSet fields + the base Merge::is_empty() trait, so no new public rs-platform-wallet symbol is added. PROJ-006: contacts/identities/asset_locks ::load_all became genuinely unused once load() returned the base ClientStartState (no callers in src or tests). Deleted all three uniformly — only the dormant, test-covered load_state readers remain (tc_p4_008/008b/008c still exercise them). platform_addrs::load_all stays (live load() path). Removed the history-narrating tombstone comment and trimmed the now- dead bulk-scan entries from the compile-time prepare allowlist. PROJ-002: rewrote the storage README "load() reconstruction" section — it still documented the rejected design (corruption tolerance, skipped_rows, partial Ok, #[non_exhaustive] ClientStartState). Now states the shipped reality: fail-hard typed errors on any corrupt row, base ClientStartState (platform_addresses + empty wallets), load_all as a wallet_meta::list_ids -> load_state loop. Scanned the other crate docs — no further stale claims. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PROJ-007: asset_locks.rs `decode_row` / `list_active` rustdoc still linked the deleted `load_all`; repointed to the surviving `load_state`. PROJ-008: identities.rs `load_state` linked the unimported `IdentityManagerStartState`; qualified it to `platform_wallet::changeset::IdentityManagerStartState`. Cheap-insurance scan of the storage crate also fixed two pre-existing strays: wallet_meta.rs `parse_network` linked the private `network_to_str` (now a plain code span), and identity_keys.rs linked the private `IdentityKeyWire` (now a plain code span — kept token-free so the secrets_scan schema guard stays green). `cargo doc -p platform-wallet-storage --no-deps` with `-D rustdoc::broken_intra_doc_links` is now exit 0. The remaining broken links reported for `-p platform-wallet` are pre-existing in files this PR never modified (byte-identical to base 738091f, zero overlap with the changed-file set) and are intentionally out of scope for this persister rework. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7a6e301 to
e999fd1
Compare
ae6b4af
into
feat/platform-wallet-sqlite-persistor
Issue being fixed or feature implemented
Follow-up PR layered on top of #3625 (
feat/platform-wallet-sqlite-persistor).Backports three improvements from the parallel persistor in dash-evo-tool's
feat/platform-wallet2branch.Backports landed:
prepare_cachedfor hot-path SQL writers (perf)load()reconstruction:platform_addressesrehydrates atboot; per-area
identities/contacts/asset_locksreaders land as hardenedfail-hard helpers, surfaced via the structured load summary log
Out of scope (deliberate):
networkcolumn for multi-network in one DB) — use separate DB files per network.ClientStartState.walletsrehydration — needs an upstreamkey_wallet::Wallet::from_persistedconstructor that does not yet exist; trackedvia
tracing::info!(wallets_pending_rehydration = N).ClientStartStateextension — proposed in an earlier revision, reverted perreview thread feat(wallet-lib): do not sync transactions if mnemonic is absent #7.
rs-platform-walletis byte-identical to the base branch.What was done?
Persister backport contained entirely within
platform-wallet-storage.rs-platform-walletis unchanged versus base (verified:git diff <base>..HEAD -- packages/rs-platform-walletis empty).Notable design decisions:
BufferAPI additive:take_for_flush+restorepreserve staged data ontransient flush failure.
Buffer::drainretained as#[deprecated]alias(removal target: 3.2.0).
WalletStorageError::FlushRetryableis the typed signal forSQLITE_BUSY/SQLITE_LOCKED;Displaystarts with the literal"FlushRetryable: "forlog-grep at the
PersistenceError::Backend(String)boundary.WalletStorageError::is_transient/error_kind_struse wildcard-freematch;the enum intentionally does NOT carry
#[non_exhaustive]so every futurevariant is forced through compile-time classification.
and
SqlitePersister::load()propagates a typedWalletStorageErroron thefirst bad row. No warn-skip, no partial
Ok, noskipped_rows.wallet_meta::list_ids → load_stateloops (threads feat: enable mainnet for dashmate #2,docs: improved sidebar and usage in DAPI client #3, chore: update webpack to version 5 in DPP #4); the duplicated bulk-decode bodies are gone.
LoadIncompleteerror variant and all references removed (thread docs(sdk): provide getTransactionHistory #5).persister.rs— no net-new publicrs-platform-walletsymbol (thread feat(wallet-lib): do not sync transactions if mnemonic is absent #7).test-helpers→__test-helpers.How Has This Been Tested?
cargo test -p platform-wallet -p platform-wallet-storage --all-features→ 227 passed, 0 failed, 3 ignored
cargo test --doc -p platform-wallet -p platform-wallet-storage --all-features→ 1 passed (
SqlitePersister::load()doctest)cargo clippy -p platform-wallet -p platform-wallet-storage --all-targets --all-features -- -D warnings→ cleancargo fmt --all -- --check→ cleanNew coverage in
sqlite_load_reconstruction.rs(tc_p4_008/008b/008c/008dassert typed
Erron corrupt rows + intact-wallet positive cases),sqlite_error_classification.rs,sqlite_buffer_semantics.rs, andsqlite_compile_time.rs(tc_p4_011compile-time-asserts the baseClientStartStateshape).Breaking Changes
None. An earlier revision extended
ClientStartState; reverted per reviewthread #7.
git diff <base>..HEAD -- packages/rs-platform-walletis empty —the crate's public surface is byte-identical to the base branch. No
BREAKING CHANGE:footer.Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent